efi: change how HostEnvironment overrides EFI variables #312
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrisccoulson
changed the title
The HostEnvironment interface contains a ReadVar method for abstracting reads of EFI variables. The default environment just calls efi.ReadVariable, with other implementations providing mocked variables. The github.com/canonical/go-efilib package provides a lot more APIs now that read from (and some that write to) EFI variables and some of these will be used from the new efi/preinstall package (these APIs are mostly boot and secure boot related). I wanted a way to be able to override the environment for the preinstall.RunChecks call (particularly as this is very useful for testing), but the existing HostEnvironment approach doesn't work well for this. I initially considered a couple of options: - Make it possible to mock each of the individual calls to go-efilib functions via the HostEnvironment interface. - Export varsBackend from go-efilib and provide a function in to mock the backend by overriding the system one. The first approach scales poorly, so my initial approach was going to be the second one. The problem with this though is that HostEnvironment will be part of a public, production API, and mocking the backend is global to the whole process. I don't think this is appropriate in production code where it might be possible that there may be one goroutine reading EFI variables from the system when another goroutine decides to mock the backend based on a supplied HostEnvironment. I've taken a different approach instead, by modifying every go-efilib function that interacts with EFI variables to accept a context.Context argument. The context contains a VarsBackend keyed from it, and the package exports a DefaultVarContext symbol corresponding to the default system backend (efivarfs if it is detected, or a null backend if it isn't detected). This approach permits individual call sites to override the backend to use for individual functions that interact with EFI variables (not just ReadVariable, WriteVariable and ListVariables, but there's a whole set of extra functions related to secure boot and boot configuration that make use of reading and writing EFI variables). The ReadVar method on the HostEnvironment interface has gone, and a new method that returns a context has been added. The internal.DefaultEnv implementation just returns efi.DefaultVarContext. The MockVars type in internal/efitest has been updated to implement efi.VarsBackend, and the MockHostEnvironment implementation in internal/efitest returns a context that keys to this implementation. This should hopefully allow preinstall.RunChecks to be able to take a HostEnvironment argument and benefit from mocked variables even when interacting with variables indirectly using higher level functions exported from go-efilib.
chrisccoulson
force-pushed
the
efi-mock-vars-backend-with-hostenv
branch
from
9474e8f to
aaba2c5
Compare
chrisccoulson
force-pushed
the
efi-mock-vars-backend-with-hostenv
branch
from
aaba2c5 to
12ec80f
Compare
pedronis
reviewed
Jul 5, 2024
efi/env.go
Outdated
| // VarContext returns a context containing a VarsBackend, keyed by efi.VarsBackendKey, | ||
| // for interacting with EFI variables via go-efilib. This context can be passed to any | ||
| // go-efilib function that interacts with EFI variables. | ||
| VarContext() context.Context |
There was a problem hiding this comment.
this is a bit a strange API for contexts, if a caller gets a context it usually should pass it along, and not start with a fresh one, so a more expected API would be:
VartContext(parent context.Context) context.Context
which would return something derived from parent (and panic if parent is nil) instead of returning a fresh context in all cases
otherwise in some scenarios you would end up having to pass more than 1 context.Context to a function which is not how context is usually meant to be used.
There was a problem hiding this comment.
Funnily enough, I'd already added an API for this, so I've just pulled in the go-efilib version with it and used it in the default implementation.
pedronis
reviewed
Jul 5, 2024
efi/internal/default_env.go
Outdated
| return readVar(name, guid) | ||
| // VarContext implements [HostEnvironment.VarContext]. | ||
| func (e defaultEnvImpl) VarContext() context.Context { | ||
| return efi.DefaultVarContext |
There was a problem hiding this comment.
so this really should really be some form of WithDefaultVarContext from efi
There was a problem hiding this comment.
Yes, I've updated this now.
chrisccoulson
force-pushed
the
efi-mock-vars-backend-with-hostenv
branch
from
12ec80f to
35d03b3
Compare
pedronis
reviewed
Jul 9, 2024
| // backend will support this eventually for variable writes because it currently implements | ||
| // a retry loop that has a potential to race with other processes. Cancelation and / or | ||
| // deadlines for sections of code that performs multiple reads using efivarfs may be useful | ||
| // in the future because the kernel rate-limits reads per-user. |
There was a problem hiding this comment.
should the doc say something about parent?
There was a problem hiding this comment.
I've updated the documentation now.
| ctx := DefaultEnv.VarContext(context.Background()) | ||
| c.Assert(ctx, NotNil) | ||
|
|
||
| expected := efi.WithDefaultVarsBackend(context.Background()) |
There was a problem hiding this comment.
maybe a good idea to pass something with an attached value already and check it can be retrieved?
There was a problem hiding this comment.
I've added this to the test
| // backend will support this eventually for variable writes because it currently implements | ||
| // a retry loop that has a potential to race with other processes. Cancelation and / or | ||
| // deadlines for sections of code that performs multiple reads using efivarfs may be useful | ||
| // in the future because the kernel rate-limits reads per-user. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
HostEnvironmentinterface contains aReadVarmethod for abstractingreads of EFI variables. The default environment just calls
efi.ReadVariable, with other implementations providing mocked variables.The
github.com/canonical/go-efilibpackage provides a lot more APIs nowthat read from (and some that write to) EFI variables and some of these
will be used from the new
efi/preinstallpackage (these APIs are mostlyboot and secure boot related). I wanted a way to be able to override the
environment for the
preinstall.RunCheckscall (particularly as this isvery useful for testing), but the existing
HostEnvironmentapproach doesn'twork well for this.
I initially considered a couple of options:
functions via the
HostEnvironmentinterface.varsBackendfrom go-efilib and provide a function in to mockthe backend by overriding the system one.
The first approach scales poorly, so my initial approach was going to be
the second one. The problem with this though is that
HostEnvironmentwill be part of a public, production API, and mocking the backend is global
to the whole process. I don't think this is appropriate in production code
where it might be possible that there may be one goroutine reading EFI
variables from the system when another goroutine decides to mock the backend
based on a supplied
HostEnvironment.I've taken a different approach instead, by modifying every go-efilib
function that interacts with EFI variables to accept a
context.Contextargument. The context contains a
VarsBackendkeyed from it, and the packageexports a
DefaultVarContextsymbol corresponding to the default systembackend (
efivarfsif it is detected, or a null backend if it isn'tdetected). I've used a context rather than passing the
VarsBackenddirectlybecause the default efivarfs backend may make use of the optional
cancellation and deadline during writing, as it runs a retry loop that potentially
races with other applications.
This approach permits individual call sites to override the backend to
use for individual functions that interact with EFI variables (not just
ReadVariable,WriteVariableandListVariables, but there's a whole setof extra functions related to secure boot and boot configuration that
make use of reading and writing EFI variables).
The
ReadVarmethod on theHostEnvironmentinterface has gone, and a newmethod that returns a context has been added. The
internal.DefaultEnvimplementation just returns
efi.DefaultVarContext.The
MockVarstype ininternal/efitesthas been updated to implementefi.VarsBackend, and theMockHostEnvironmentimplementation ininternal/efitestreturns a context that keys to this implementation.This should hopefully allow
preinstall.RunChecksto be able to take aHostEnvironmentargument and benefit from mocked variables even wheninteracting with variables indirectly using higher level functions
exported from go-efilib.